Skip to content

Conversation

jrenaat
Copy link
Member

@jrenaat jrenaat commented Jul 22, 2025


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


https://hibernate.atlassian.net/browse/HHH-19631

@jrenaat jrenaat requested review from beikov and gavinking July 22, 2025 16:41
Copy link
Member

@gavinking gavinking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrenaat This is good, but not quite enough. We get a similar NPE for:

parent.getChildren().iterator().next();

Also, the error message is a little too specific. I think the error could occur in other situations, e.g. whenever the collection is unreferenced.

@jrenaat jrenaat force-pushed the HHH-19631_NPE-predeletelistener branch from 2404953 to 29c6433 Compare July 22, 2025 23:02
@jrenaat jrenaat requested a review from gavinking July 22, 2025 23:02
@jrenaat jrenaat force-pushed the HHH-19631_NPE-predeletelistener branch from 29c6433 to 2160996 Compare July 22, 2025 23:06
@jrenaat
Copy link
Member Author

jrenaat commented Jul 22, 2025

@jrenaat This is good, but not quite enough. We get a similar NPE for:

parent.getChildren().iterator().next();

Added that to tghe test

Also, the error message is a little too specific. I think the error could occur in other situations, e.g. whenever the collection is unreferenced.

Tweaked the message a bit, let me know if it's better now.
I tried to get the error by dereferencing the collection, but couldn't, maybe I'm not doing it correctly, not sure.

@jrenaat jrenaat force-pushed the HHH-19631_NPE-predeletelistener branch from 2160996 to c8d4d06 Compare July 23, 2025 15:49
Comment on lines 659 to 667
public static void checkPersister(PersistentCollection collection, CollectionPersister persister) {
if ( !collection.wasInitialized() && persister == null ) {
((AbstractPersistentCollection) collection).throwLazyInitializationException( "collection is being removed" );
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this method really need to be static?

Copy link
Member Author

@jrenaat jrenaat Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to add a default method to the PersistentCollection interface? Even if it's marked as @Internal, it's still poorly justified i think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I was asking if it can be an instance method so that you don't need to pass in the PersistentCollection collection and then cast it to AbstractPersistentCollection.

Copy link
Member Author

@jrenaat jrenaat Jul 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what i mean, if we don't add it to the super interface, we end up having to cast in DefaultInitializeCollectionEventListener, not sure which is better, but now at least it's contained so to speak?

@jrenaat jrenaat force-pushed the HHH-19631_NPE-predeletelistener branch from c8d4d06 to 1c4384b Compare July 24, 2025 15:39
@gavinking gavinking merged commit 8b7947d into hibernate:main Jul 25, 2025
39 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants